fix: raise ValueError when no valid estimand is identified instead of silent None return#1552
Conversation
…f silent None return Signed-off-by: abhiprd2000 <8292aniarc@gmail.com>
Signed-off-by: abhiprd2000 <8292aniarc@gmail.com>
|
Hi @emrekiciman, follow-up to your suggestion on #1520. Fix and regression test included. |
Signed-off-by: abhiprd2000 <8292aniarc@gmail.com>
|
Hi @emrekiciman, Noticed github-actions bot also filed a draft #1554 for the same fix. Mine includes an end-to-end test via |
There was a problem hiding this comment.
Pull request overview
This PR makes estimate_effect() fail loudly when identification did not produce a valid estimand for the requested identifier, by raising a ValueError instead of returning a CausalEstimate with a None value. This aligns behavior with the expectation that a missing estimand indicates identification failure rather than a valid “no effect” result.
Changes:
- Raise a
ValueError(with identifier name included) whenidentified_estimand.estimands[identifier_name] is None. - Add a regression test covering the missing-estimand case for
iv.instrumental_variable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
dowhy/causal_estimator.py |
Replaces the silent CausalEstimate(None, …) return with a logged ValueError when the estimand is missing. |
tests/test_causal_estimator.py |
Adds a regression test asserting estimate_effect() raises ValueError for missing IV estimand. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Abhimanyu Prasad <8292aniarc@gmail.com>
amit-sharma
left a comment
amit-sharma
left a comment
There was a problem hiding this comment.
thanks @abhiprd2000 This looks good
|
@all-contributors add @abhiprd2000 for code |
|
I've put up a pull request to add @abhiprd2000! 🎉 |
|
Hi @amit-sharma and @emrekiciman, I tried syncing the branch with main, but the automated web merge appears to have introduced a case-sensitivity/import mapping mismatch with Thanks again! |
|
#1573 to fix the propensity balance interpreter error |
|
Brilliant, thank you @emrekiciman for the incredibly fast turnaround on both infrastructure hotfixes (#1572, #1573). |
Summary
estimate_effect()silently returnsNonewhenidentified_estimand.estimands[identifier_name]isNone, logging an error but not raising. Users get no indication that identification failed.Fix
Replace the silent
return CausalEstimate(None, ...)withraise ValueError(error_msg)including the identifier name for easier debugging.Impact
ValueErrorinstead of silentNoneFixes #1551
Signed-off-by: abhiprd2000